-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add GAP.Packages.versioninfo()
#1119
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1119 +/- ##
==========================================
- Coverage 75.27% 74.58% -0.70%
==========================================
Files 55 55
Lines 4668 4713 +45
==========================================
+ Hits 3514 3515 +1
- Misses 1154 1198 +44
|
Looks nice! |
My point of view is that |
I would vote for |
Concerning the jll information:
The longest line of this table is 107 characters wide. |
We could event keep |
- change `GAP.Packages.versioninfo`: do not show the GAP version, show GAP packages (versions and paths) and jll packages in separate lists, similar to Oscar's `versioninfo`, - do not show `*` in the beginning of the lines for those packages whose paths are nonstandard, - add `GAP.versioninfo` that shows also the GAP (and GAP_jll) version, - introduce keyword arguments for including jll and GAP information, - document `GAP.versioninfo` not `GAP.Packages.versioninfo`.
julia> GAP.versioninfo()
ERROR: BoundsError: attempt to access 0-element Vector{Pkg.API.PackageInfo} at index [1]
Stacktrace:
[1] throw_boundserror(A::Vector{Pkg.API.PackageInfo}, I::Tuple{Int64})
@ Base ./essentials.jl:14
[2] getindex
@ ./essentials.jl:916 [inlined]
[3] versioninfo(io::Base.TTY; jll::Bool, padding::String)
@ GAP ~/code/julia/GAP.jl/src/GAP.jl:338
[4] versioninfo(io::Base.TTY)
@ GAP ~/code/julia/GAP.jl/src/GAP.jl:336
[5] top-level scope
@ REPL[2]:1 |
- show the jll version for all installed packages not only for the loaded ones, since the jlls are loaded - add `full` argument for `GAP.versioninfo`, like in Oscar - add a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three more comments, otherwise looks great from my POV. Thanks!
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
Resolves #1082
With this code, I get the following output after
using Oscar
.Would it make sense to show also the version of the
jll
package if applicable?(Remove the comment signs in the new code in order to see the result.)
If yes then it would make sense to show two lines per package.